Support an 'owner' key in region_highlight. #57

danielshahaf · 2020-05-03T04:47:02Z

This is to allow plugins to easily remove only entries they had added. See zsh-users/zsh-syntax-highlighting#418 and the cross-referenced issues.

danielshahaf · 2020-05-04T16:05:46Z

Doc/Zsh/zle.yo

 enditemize()

 For example, 

-example(region_highlight=("P0 20 bold"))
+example(region_highlight=("P0 20 bold owner=myplugin"))


Older zsh's parse this as equivalent to P0 20 none unless a comma is added after bold. Is there a better option?

Hold on, doesn't using a comma separator give us backward compatibility?
region_highlight=("P0 20 bold,owner=myplugin") seems to work in current zsh.

We may lose the owner in an old zsh but that only loses us the ability to prevent plugins from interfering with each other. Losing the bold is a backward compatibility problem.

It could be worth thinking about other uses for the owner field. We could add a region_precedence array which might contain, e.g. ( vi-exchange builtin z-s-h ) so z-s-h doesn't need to duplicate the code for rendering the region/selection.

Thanks for the review.

I considered using a comma separator, but (1) the owner's identity seemed like metadata, not an intrinsic part of the typographic specification; and (2) the syntax in the PR was mildly easier to implement.

I'll sleep on a numeric precedence= key (à la add-zle-hook-widget style values) and solving the region/standout duplication issue and get back to you on them.

Fair enough. Do I understand correctly that the only downside of your approach (with a space) is that for backwards compatibility, you need a trailing comma on the attributes if there isn't one, e.g. bold, I agree that it logically belongs separately so is cleaner.

Any precedence feature is a separate matter really. It just came to mind when looking at this. Rather than a numeric precedence= key, I was just thinking of an array listing owners in order. By default it'd just contain an entry for the builtin stuff. Plugins would probably add themself to either the beginning or end and users could always reorder them. There are probably other schemes that could work.

I understand correctly that the only downside of your approach (with a space) is that for backwards compatibility, you need a trailing comma on the attributes if there isn't one, e.g. bold,

It's the only downside I'm aware of, yes. But there's a reason I've posted this for review :)

Any precedence feature is a separate matter really. It just came to mind when looking at this. Rather than a numeric precedence= key, I was just thinking of an array listing owners in order. By default it'd just contain an entry for the builtin stuff. Plugins would probably add themself to either the beginning or end and users could always reorder them. There are probably other schemes that could work.

The idea being that plugin authors are better placed than users to determine the best relative order of highlights, I guess? For example, if zsh-autosuggestions were to highlight a suggestion that had just been inserted, that would probably want to apply after z-sy-h's highlights?

Compare the proof-of-concept docs patch I posted in zsh-users/zsh-autosuggestions#529 (comment) under the heading "Describe alternatives you've considered". (None of the dicsussion in the issue is about that diff, though the LASTWIDGET issue in Eric's latest comment may be of independent interest.)

danielshahaf · 2020-05-04T16:06:46Z

Src/Zle/zle_refresh.c

+	    strp++;
+
+	if (strpfx("owner=", strp))
+	    rhp->owner = ztrdup(strp + 6);


Given the above comment, this shouldn't copy the entire rest of the string, but only up to the next space, for extensibility (forward compatibility).

Should be resolved by the just-pushed commits. (The hex commit id's will be changed later when I squash them, so I'm not pointing to a specific commit.)

New tests added for this.

ericfreese · 2020-05-22T13:25:19Z

Just brainstorming a bit.

As a plugin author, setting an "owner" value is a bit more broad of a use case than I need. All I really need is to be able to uniquely identify certain highlights that I created so that I can delete them later on. If when I create a new highlight, I could get back an automatically generated unique identifier for that highlight in REPLY, that would be enough for me.

@danielshahaf what would you think of adding a new flag (maybe -h?) to the zle built-in? It would save a specified highlight and give back a unique identifier for that highlight in REPLY. The unique identifier could then be provided to remove the highlight.

local REPLY

# Add a new highlight with `zle -h`
zle -h "P0 20 bold"

# Remove the highlight by adding `-d` flag
zle -h -d $REPLY

I'm thinking that highlights added this way would not be accessible through the region_highlight variable and vice versa, but highlights from both would be applied when rendering.

danielshahaf · 2020-05-23T01:23:45Z

Just brainstorming a bit.

Thanks for the review.

As a plugin author, setting an "owner" value is a bit more broad of a use case than I need. All I really need is to be able to uniquely identify certain highlights that I created so that I can delete them later on. If when I create a new highlight, I could get back an automatically generated unique identifier for that highlight in REPLY, that would be enough for me.

Could you elaborate on why z-asug needs unique identifiers? I take it z-asug's use-case wouldn't be addressed by a hardcoding a fixed string or three, but why?

In terms of the PR as it stands, the closest equivalent to your functionality would be something along the lines of:

owner="z-asug:$RANDOM:"
region_highlight+=( "P0 20 bold owner=$owner" )

and afterwards remove it using the method described in the docs change in this PR.

(Aside: I'll change "owner" in the PR to some more generic term before posting/merging this.)

@danielshahaf what would you think of adding a new flag (maybe -h?) to the zle built-in? It would save a specified highlight and give back a unique identifier for that highlight in REPLY. The unique identifier could then be provided to remove the highlight.
local REPLY

# Add a new highlight with `zle -h`
zle -h "P0 20 bold"

# Remove the highlight by adding `-d` flag
zle -h -d $REPLY
I'm thinking that highlights added this way would not be accessible through the region_highlight variable and vice versa, but highlights from both would be applied when rendering.

On the one hand, encapsulating the details is appealing. I suspect, however, that a blackbox that only supports "add a highlight" and "delete a highlight given its nonce" would be a bit too specialized.

On the other hand, the approach I described above seems to me to be more closely aligned with PEP 20's principles and to be more generic/flexible. Indeed, it would be easy to implement your proposed API around the API in the PR:

zle-h() {
    if [[ $1 == -d ]]; then
        region_highlight=( ${region_highlight:#*owner="$2"} )
    else
        setopt localoptions nowarncreateglobal
        REPLY="$RANDOM.$RANDOM"
        region_highlight+=( "$1 owner=$REPLY" )
    fi
}

… whereas the converse would require callers to maintain their own records of which nonces (returned values of $REPLY) identify which highlights. For example, z-sy-h would have to maintain a simple array of all nonces it has set, and then delete all of them (perhaps using a zle -h -d "${nonces[@]}" syntax). A plugin that needs to distinguish among its own highlights — z-sy-h doesn't — would have to maintain multiple arrays, or an associative array, etc., to track its nonces.

Not sure which I prefer, yet.

ericfreese · 2020-05-23T04:39:54Z

Could you elaborate on why z-asug needs unique identifiers? I take it z-asug's use-case wouldn't be addressed by a hardcoding a fixed string or three, but why?

The need for unique identifiers is just so that I can be sure that I’m not removing anything else when I delete my highlight. If it weren’t unique, there may be a chance that I remove something else unintentionally. Hard coding a fixed string would work practically but my point was that it’s not functionality that I need. I don’t care what the identifier value is, so having to specify some value for it is just extra work for me to do. It would be nice if zsh could handle it for me. It’s not a big deal, just a thought while we’re coming up with new functionality 😄

I'm thinking that highlights added this way would not be accessible through the region_highlight variable and vice versa, but highlights from both would be applied when rendering.

As I think about this more I’m wondering if would be feasible to just store the highlights added via the new zle flag to region_highlight. The unique identifier would then be just an optional attribute of a highlight- present for those added via zle and not present for those added through setting the param.

danielshahaf · 2020-05-23T05:19:26Z

> Could you elaborate on why z-asug needs unique identifiers? I take it z-asug's use-case wouldn't be addressed by a hardcoding a fixed string or three, but why? The need for unique identifiers is just so that I can be sure that I’m not removing anything else when I delete my highlight. If it weren’t unique, there may be a chance that I remove something else unintentionally. Hard coding a fixed string would work practically but my point was that it’s not functionality that I need. I don’t care what the identifier value is, so having to specify some value for it is just extra work for me to do. It would be nice if zsh could handle it for me. It’s not a big deal, just a thought while we’re coming up with new functionality 😄

WDYT of, say, «region_highlight+=( "P0 20 bold autoremove=yes" )» to add an entry that would be automatically removed (by add-zle-hook-widget or possibly zle itself) at some appropriate time? (What would an appropriate time be in your use-case?) I'll sleep on the rest of your comment and reply further later.

danielshahaf · 2020-05-23T18:08:47Z

As I think about this more I’m wondering if would be feasible to just store the highlights added via the new zle flag to region_highlight. The unique identifier would then be just an optional attribute of a highlight- present for those added via zle and not present for those added through setting the param.

Okay, so you're proposing to encapsulate adding a single entry and removing a single entry. However, operations such as "remove all my entries", "remove all entries", "enumerate entries", etc., would not be encapsulated, so they'd only be possible to implement by directly accessing the array. Doesn't that somewhat negate the benefits of encapsulation, at least at the zle level? An encapsulation, in this case, would seem to be another API entry point to maintain, but without the benefit of being able to change the implementation without changing the API callers?

ericfreese · 2020-05-28T14:23:30Z

WDYT of, say, «region_highlight+=( "P0 20 bold autoremove=yes" )» to add
an entry that would be automatically removed (by add-zle-hook-widget or
possibly zle itself) at some appropriate time? (What would an
appropriate time be in your use-case?)

Hmm yeah this could probably work. The appropriate time for z-asug could probably be any time between the end of a redraw and the start of the next redraw cycle (just before the line-pre-redraw hooks are called). This would probably be the right time for z-sy-h as well. So setting that flag would be kind of like saying "I want this highlight to be around for the next redraw cycle, but remove it after that".

I'm a little apprehensive to give up control of when exactly the entries are removed, but I'm struggling to come up with any real justification for that apprehension. Would there be cases where someone would want a highlight to stick around until they manually remove it?

Okay, so you're proposing to encapsulate adding a single entry and removing a single entry. However, operations such as "remove all my entries", "remove all entries", "enumerate entries", etc., would not be encapsulated, so they'd only be possible to implement by directly accessing the array.

That's basically what I'm thinking. You could still support "remove all my entries" by passing multiple tokens to zle -h -d. And if there's some way to return multiple entries in REPLY, we could add multiple entries using zle -h as well. "remove all entries" could be supported by allowing a pattern arg to zle -h -d and passing something like zle -h -d *. "enumerate entries" could be zle -h -l or something. So I think we could wrap the whole thing up in the zle interface, but it will be a bit more clunky for those use cases where the param makes more sense.

Doesn't that somewhat negate the benefits of encapsulation, at least at the zle level?

Somewhat yeah. The benefit we'd get from adding zle -h is being able to have side effects when a highlight is added (setting REPLY to a unique identifier for that highlight). I'm thinking we keep region_highlight around in its current form mainly for backward compatibility, but there are some other options:

Deprecate region_highlight and encapsulate highlight behavior entirely in new zle -h.
Make region_highlight read-only and encapsulate adding/removing highlights in zle -h. (Isn't this kind of like existing params like $widgets that are modified by builtins but available as read-only params for convenience?)

An encapsulation, in this case, would seem to be another API entry point to maintain, but without the benefit of being able to change the implementation without changing the API callers?

I think the implementation could technically change as long as the API contracts are kept intact. The API contracts being the details of both zle -h builtin and region_highlight param. For example, the algorithm to generate the unique token for each new zle -h highlight could be altered without any changes required to clients.

danielshahaf · 2020-05-28T18:43:32Z

WDYT of, say, «region_highlight+=( "P0 20 bold autoremove=yes" )» to add an entry that would be automatically removed (by add-zle-hook-widget or possibly zle itself) at some appropriate time? (What would an appropriate time be in your use-case?)

Hmm yeah this could probably work. The appropriate time for z-asug could probably be any time between the end of a redraw and the start of the next redraw cycle (just before the line-pre-redraw hooks are called). This would probably be the right time for z-sy-h as well.

Yes.

So setting that flag would be kind of like saying "I want this highlight to be around for the next redraw cycle, but remove it after that".

I'm a little apprehensive to give up control of when exactly the entries are removed, but I'm struggling to come up with any real justification for that apprehension. Would there be cases where someone would want a highlight to stick around until they manually remove it?

Who's "someone"? A user? A plugin author? Whoever it is, couldn't they add their entry without autoremove=yes (or remove the autoremove=yes annotation from an existing entry)?

If you're getting at making autoremove=yes the default, wouldn't that be backwards incompatible?

Okay, so you're proposing to encapsulate adding a single entry and removing a single entry. However, operations such as "remove all my entries", "remove all entries", "enumerate entries", etc., would not be encapsulated, so they'd only be possible to implement by directly accessing the array.

That's basically what I'm thinking. You could still support "remove all my entries" by passing multiple tokens to zle -h -d. And if there's some way to return multiple entries in REPLY, we could add multiple entries using zle -h as well.

There is: by convention, $REPLY is a scalar and $reply is an array.

"remove all entries" could be supported by allowing a pattern arg to zle -h -d and passing something like zle -h -d *. "enumerate entries" could be zle -h -l or something. So I think we could wrap the whole thing up in the zle interface, but it will be a bit more clunky for those use cases where the param makes more sense.

It's certainly possible to encapsulate these other operations as well.

Doesn't that somewhat negate the benefits of encapsulation, at least at the zle level?

Somewhat yeah. The benefit we'd get from adding zle -h is being able to have side effects when a highlight is added (setting REPLY to a unique identifier for that highlight).

Do you see any other use-cases besides setting $REPLY as mentioned?

I'm thinking we keep region_highlight around in its current form mainly for backward compatibility, but there are some other options:

Deprecate region_highlight and encapsulate highlight behavior entirely in new zle -h.

Make region_highlight read-only and encapsulate adding/removing highlights in zle -h. (Isn't this kind of like existing params like $widgets that are modified by builtins but available as read-only params for convenience?)

So, the code you're proposing is:

local s="P0 20 bold"
zle -h -- $s
typeset -g _z_asug_cookie=$REPLY

and the code I'm proposing is:

local s="P0 20 bold"
typeset -g _z_asug_cookie=:z-asug:$RANDOM:
region_highlight+=( "$s owner=$_z_asug_cookie" )

And the main difference is that your proposal involves designing, implementing, and documenting a new API. If the old API remains fully supported, the first proposal would increase zle's maintenance burden; and if we don't, the existing UI would have to be deprecated [which has a cost in man-hours too], and subsequently plugin maintainers would have to update their code.

I am sorry, but I do not consider separating the generation-of-nonces concern from z-asug to be sufficient justification for imposing these costs on zle and other plugins.

What's so foreboding about the second alternative? All you said was:

I don’t care what the identifier value is, so having to specify some value for it is just extra work for me to do.

I see that that the second alternative's design is not in the first normal form, and that it allows stuff like random write access (region_highlight[$i]=$foo), both of which we might have done differently if we'd been designing this from scratch — but we aren't designing this from scratch. We're designing a change to an existing API with an existing ecosystem of API consumers around it. In such cases, it's good practice to opt for small changes in preference to massive rewrites, where possible.

If it helps, we could add some sort of magic parameter that expands to a different value every time it's referenced: perhaps $AUTOINCREMENT (expands to 0, then 1, then 2, then 3, …) or $NONCE (value only guaranteed to be unique; maybe $UUIDGEN after uuidgen(1)).

Or you could propose that the zle-h (sic) function I posted a few comments ago be cleaned up, extended, and added to the distribution as a standard autoloadable function. Something like that could be useful, because it could solve the "not in the first normal form" problem centrally, for both reads and writes. It could also take care of inventing nonces.

An encapsulation, in this case, would seem to be another API entry point to maintain, but without the benefit of being able to change the implementation without changing the API callers?

I think the implementation could technically change as long as the API contracts are kept intact. The API contracts being the details of both zle -h builtin and region_highlight param. For example, the algorithm to generate the unique token for each new zle -h highlight could be altered without any changes required to clients.

Well, yes, that is an example of encapsulating an implementation detail. What I fail to see is why that detail needs to be encapsulated in the first place. Do you expect best practices in the area of nonce-generation to change in the coming years?

Next steps:

Sound the mailing list about owner=$owner v. autoremove=yes.
@ericfreese What's so foreboding about that second code snippet above?

Cheers!

okapia · 2020-05-29T06:03:08Z

(Aside: I'll change "owner" in the PR to some more generic term before posting/merging this.)

"id" perhaps. Is nice and short. Could document a convention of prefixing it with a string to identify the owning plugin.

okapia · 2020-05-29T06:12:54Z

Would there be cases where someone would want a highlight to stick around until they manually remove it?

Most definitely yes. Most of my uses of region_highlight fall into this category. The vi-exchange plugin of mine that is public needs the highlight to stay around.

An autoremove, entry seems reasonable. (the shorter name might be nice). Could be good to have the value make it clear what the remove criteria would be - also useful for future extensions, e.g. autoremove=key for when another key is pressed. Or even autoremove=0.5 for a ½ second.

okapia · 2020-05-29T06:20:10Z

I can't say I really like the zle -h suggestion. I like the visibility and transparency that comes with having something I already understand and know how to manipulate like a variable. And I don't see how it really makes this any easier.

If it generates an id for you in $REPLY, a plugin that lives from one widget invocation to the next needs to store that id in a global variable. And as global variable names clash, what have you gained over hardcoding your own ids. And they'll be less meaningful, I'd rather see id=vi-exchange:selected than id=23fe1ca.

danielshahaf · 2020-05-29T16:21:50Z

An autoremove, entry seems reasonable. (the shorter name might be nice).

What "shorter name" are you referring to? None has been proposed.

Could be good to have the value make it clear what the remove criteria would be - also useful for future extensions, e.g. autoremove=key for when another key is pressed. Or even autoremove=0.5 for a ½ second.

+1

danielshahaf · 2020-05-29T16:25:41Z

"id" perhaps. Is nice and short. Could document a convention of prefixing it with a string to identify the owning plugin.

I was thinking of rhid (initialism of "region_highlight id" ) for greppability reasons.

okapia · 2020-05-30T01:31:16Z

What "shorter name" are you referring to? None has been proposed.

That was a typo or similar. I meant a not the.

Ideas that come to mind include, "expire", "expiry", "lifetime", "remove", "scope", "transient", "tmp"

okapia · 2020-05-30T01:36:48Z

I was thinking of rhid (initialism of "region_highlight id" ) for greppability reasons.

I'm not especially fond of such identifying initialisms. It's only used within region_highlight. For grep, region_highlight.*id ought to work often enough (barring word wrapping) and for a particular plugin, id=z-s-h may work.

danielshahaf · 2020-05-30T02:57:27Z

Ideas that come to mind include, "expire", "expiry", "lifetime", "remove", "scope", "transient", "tmp"

expiry sounds right. It's a somewhat more abstract than autoremove.

I'm not especially fond of such identifying initialisms. It's only used within region_highlight. For grep, region_highlight.*id ought to work often enough (barring word wrapping) and for a particular plugin, id=z-s-h may work.

In my experience, self-descriptive names are useful. See, for example, https://subversion.apache.org/issue3744. That's why I'm not that fond of bare id.

But I get that rhid is a bit cryptic. Kinda comes with the territory of being short, though. Anyway, how about, say, stroke or stroke_id?

Do we need the owner= functionality at all if we have the autoremove (or expiry) functionality?

Are there any outstanding concerns with the functionality, other than the names of the API entry points?

okapia · 2020-05-30T22:53:01Z

In my experience, self-descriptive names are useful. See, for example, https://subversion.apache.org/issue3744. That's why I'm not that fond of bare id.

With the scope limited to the region_highlight array, I can't see there being any confusion.

But I get that rhid is a bit cryptic. Kinda comes with the territory of being short, though. Anyway, how about, say, stroke or stroke_id?

"stroke" is too suggestive of a movement in it's meaning which I don't think applies here. It may have subtle US/UK differences in meaning. How about "mark". A thesaurus also turns up "tag", "stamp", "label" and "badge". In a somewhat different vein, "memo" or "key".

Do we need the owner= functionality at all if we have the autoremove (or expiry) functionality?

Checking back over the discussion, I don't think you exactly fleshed out when auto-removal would occur. It perhaps removes the need from z-s-h and any widget that re-applies things any time the buffer changes. But for longer persisting things like vi-exchange, it'd still be useful.

danielshahaf · 2020-05-31T01:20:37Z

> In my experience, self-descriptive names are useful. See, for example, https://subversion.apache.org/issue3744. That's why I'm not that fond of bare `id`. With the scope limited to the region_highlight array, I can't see there being any confusion.

Yeah, but a unique name would be more greppable. `grep -R rhid` would in practice have near-100% specificity and sensitivity; that's not the case for either `grep -Rw id` or `grep -R 'region_highlight.*id'`. [This goes not only for `rhid` but also for any other sufficiently-unique term.]

> But I get that `rhid` is a bit cryptic. Kinda comes with the territory of being short, though. Anyway, how about, say, `stroke` or `stroke_id`? "stroke" is too suggestive of a movement in it's meaning which I don't think applies here. It may have subtle US/UK differences in meaning. How about "mark". A thesaurus also turns up "tag", "stamp", "label" and "badge". In a somewhat different vein, "memo" or "key".

I was thinking of the noun sense of 'stroke', but yeah, something else. 'label' and 'memo' both sound good.

> Do we need the `owner=` functionality at all if we have the `autoremove` (or `expiry`) functionality? Checking back over the discussion, I don't think you exactly fleshed out when auto-removal would occur.

Autoremoval should take place at the start of the following redraw cycle (the cycle _after_ the one zle-line-pre-redraw is running as part of), I suppose?

It perhaps removes the need from z-s-h and any widget that re-applies things any time the buffer changes. But for longer persisting things like vi-exchange, it'd still be useful.

Ack usefulness. So, I think the subtotal up to this point is to add both memo= (née owner=) and expiry=next-redraw (née autoremove=yes)? If so, let's take this proposal to -workers@.

ericfreese · 2020-05-31T13:22:01Z

If you're getting at making autoremove=yes the default, wouldn't that be backwards incompatible?

No, was just wondering if we would still need the "owner" solution. Sounds like we do.

Do you see any other use-cases besides setting $REPLY as mentioned?

No that's the only one I'm seeing for now.

I see that that the second alternative's design is not in the first normal form, and that it allows stuff like random write access (region_highlight[$i]=$foo), both of which we might have done differently if we'd been designing this from scratch — but we aren't designing this from scratch. We're designing a change to an existing API with an existing ecosystem of API consumers around it. In such cases, it's good practice to opt for small changes in preference to massive rewrites, where possible.

Makes sense. I figured this would be the outcome but thought I'd give my 2 cents anyway.

If it helps, we could add some sort of magic parameter that expands to a different value every time it's referenced: perhaps $AUTOINCREMENT (expands to 0, then 1, then 2, then 3, …) or $NONCE (value only guaranteed to be unique; maybe $UUIDGEN after uuidgen(1)).

This is an interesting idea. Probably not necessary for now, but I'll keep it in mind for the future.

What's so foreboding about that second code snippet above?

Nothing in particular. I was just thinking it might be a nice feature to implement so that clients (plugins, users, etc.) wouldn't need to worry about implementing the nonce-generation themselves.

I think the subtotal up to this point is to add both memo= (née owner=) and expiry=next-redraw (née autoremove=yes)?

Do we need both? I would be fine with just memo/owner for now.

danielshahaf · 2020-06-01T08:49:02Z

> I think the subtotal up to this point is to add both memo= (née owner=) and expiry=next-redraw (née autoremove=yes)? Do we need both? I would be fine with just memo/owner for now.

Good point. Yes, let's add just memo=, then. Unless implementing expiration in zle itself would be a noticeable performance gain? (@psprint?)

danielshahaf · 2020-06-06T08:45:57Z

Thread: workers/46001

[s/owner=/memo=/g] Support a 'memo' key in region_highlight. This is to allow plugins to easily remove only entries they had added. See zsh-users/zsh-syntax-highlighting#418 and the cross-referenced issues. This was initially worked on at zsh-users#57, where it was initially called "owner=".

[Remove a magic number]

Incomplete; see test. Fixed in the next commit.

…ort (see tests).

danielshahaf · 2020-06-17T10:32:49Z

Pushed some commits that rename owner= to memo= and implement forwards compatibility. I'll probably squash the lot before I post to -workers@, but I'm posting them here unsquashed for ease of review for those who have reviewed the previous (first) revision.

danielshahaf · 2020-06-17T10:34:10Z

Two TODO comments remain:

+           /* ### TODO: Consider optimizing the common case that memo_start to
+            *           end-of-string is entirely ASCII */
+           while (1) {

+           /* TODO: save rhp->memo here? */
            if (zlemetaline) {
                newrhp->start = rhp->start_meta;

danielshahaf · 2020-06-18T08:59:03Z

Src/Zle/zle_refresh.c

+		} else
+		    i += nbytes;
+	    }
+	    rhp->memo = ztrduppfx(memo_start, memo_end - memo_start);


Where's the corresponding free()?

This causes memos to be preserved across invocations of _cmdstring, at least (probably because _cmdstring calls `compset -q` and set_comp_sep() calls zle_save_positions()).

The allocation in zle_set_highlight() did not require an update.

danielshahaf · 2020-06-18T10:35:02Z

Posted this PR as of 23d1edc, squashed, to -workers@ in workers/46068.

danielshahaf · 2020-06-25T11:57:04Z

Merged in dd6e702. Thanks for all the reviews and testing!

I'll go back to zsh-users/zsh-syntax-highlighting#418 and see what can be done to move it forward, now.

…o= support. This is useful when multiple plugins add region_highlight entries and subsequently want to remove only their own entries. Without this functionality, recognizing one's region_highlight entries is not trivial because the 'start' and 'end' offsets are modified by editing of $BUFFER and the highlight specification may not be unique or distinctive. The tweaks are as follows: - Change zfree() to zsfree() per workers/46070. - Remove the mem.c hunk, as it changed the signature of only one out of two alternative definitions of zsfree(). (The definition that hunk touched is the one that's not used by default.)

Support an 'owner' key in region_highlight.

955caae

This is to allow plugins to easily remove only entries they had added. See zsh-users/zsh-syntax-highlighting#418 and the cross-referenced issues.

danielshahaf mentioned this pull request May 3, 2020

Avoid emptying the whole of region_highlight zsh-users/zsh-syntax-highlighting#418

Closed

danielshahaf commented May 4, 2020

View reviewed changes

This was referenced May 4, 2020

feature/redrawhook breaks zsh-autosuggestions zsh-users/zsh-syntax-highlighting#579

Closed

Release 0.8.0? zsh-users/zsh-syntax-highlighting#722

Closed

This was referenced May 19, 2020

zsh-syntax-highlighting interoperability: Please register zle-line-pre-redraw hooks zsh-users/zsh-autosuggestions#529

Open

redrawhook: Effect on third-party plugins zsh-users/zsh-syntax-highlighting#735

Closed

danielshahaf requested a review from ericfreese May 22, 2020 02:09

This was referenced May 22, 2020

_zsh_highlight_load_highlighters consumes much resources? zsh-users/zsh-syntax-highlighting#734

Closed

Canonicalize option settings earlier zsh-users/zsh-syntax-highlighting#536

Open

danielshahaf mentioned this pull request May 26, 2020

No highlighting while cycling menu completion results zsh-users/zsh-syntax-highlighting#375

Closed

danielshahaf mentioned this pull request Jun 6, 2020

Using with Zinit "Turbo mode" breaks zsh autosuggestions zsh-users/zsh-syntax-highlighting#742

Open

danielshahaf added 5 commits June 17, 2020 10:22

fixup! Support an 'owner' key in region_highlight.

0bc43f2

[Remove a magic number]

CHECKPOINT: Forward compatibility for the memo's value

140f1e4

Incomplete; see test. Fixed in the next commit.

Support metafied and multibyte characters in the memo. Fixes NUL supp…

a2b6a3f

…ort (see tests).

Document compatibility issues, sans a hacky workaround (see comments).

6e08bb9

danielshahaf added 2 commits June 17, 2020 19:14

tweak docs

8233536

tweak docs zsh-users#2

125ccbd

danielshahaf commented Jun 18, 2020

View reviewed changes

danielshahaf added 4 commits June 18, 2020 09:03

docs tweak zsh-users#3

12586a3

Save/restore memo in zle_save_positions()/zle_restore_positions().

866209d

This causes memos to be preserved across invocations of _cmdstring, at least (probably because _cmdstring calls `compset -q` and set_comp_sep() calls zle_save_positions()).

Let zsfree() take a pointer to const, per best practice.

f743943

Deallocate memos. Fixes a memory leak

adf432e

The allocation in zle_set_highlight() did not require an update.

danielshahaf force-pushed the region_highlight-owner-v1 branch from 39d4460 to adf432e Compare June 18, 2020 09:35

docs tweak zsh-users#4

23d1edc

danielshahaf closed this Jun 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support an 'owner' key in region_highlight. #57

Support an 'owner' key in region_highlight. #57

danielshahaf commented May 3, 2020

danielshahaf May 4, 2020

okapia May 21, 2020 •

edited

Loading

danielshahaf May 21, 2020

okapia May 21, 2020

danielshahaf May 21, 2020

danielshahaf May 4, 2020

danielshahaf Jun 17, 2020

danielshahaf Jun 17, 2020

ericfreese commented May 22, 2020

danielshahaf commented May 23, 2020

ericfreese commented May 23, 2020

danielshahaf commented May 23, 2020 via email

danielshahaf commented May 23, 2020

ericfreese commented May 28, 2020

danielshahaf commented May 28, 2020

okapia commented May 29, 2020

okapia commented May 29, 2020

okapia commented May 29, 2020

danielshahaf commented May 29, 2020

danielshahaf commented May 29, 2020

okapia commented May 30, 2020

okapia commented May 30, 2020

danielshahaf commented May 30, 2020

okapia commented May 30, 2020

danielshahaf commented May 31, 2020 via email •

edited

Loading

ericfreese commented May 31, 2020

danielshahaf commented Jun 1, 2020 via email

danielshahaf commented Jun 6, 2020

danielshahaf commented Jun 17, 2020 •

edited

Loading

danielshahaf commented Jun 17, 2020

danielshahaf Jun 18, 2020

danielshahaf Jun 18, 2020

danielshahaf commented Jun 18, 2020

danielshahaf commented Jun 25, 2020

Support an 'owner' key in region_highlight. #57

Support an 'owner' key in region_highlight. #57

Conversation

danielshahaf commented May 3, 2020

Choose a reason for hiding this comment

okapia May 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericfreese commented May 22, 2020

danielshahaf commented May 23, 2020

ericfreese commented May 23, 2020

danielshahaf commented May 23, 2020 via email

danielshahaf commented May 23, 2020

ericfreese commented May 28, 2020

danielshahaf commented May 28, 2020

okapia commented May 29, 2020

okapia commented May 29, 2020

okapia commented May 29, 2020

danielshahaf commented May 29, 2020

danielshahaf commented May 29, 2020

okapia commented May 30, 2020

okapia commented May 30, 2020

danielshahaf commented May 30, 2020

okapia commented May 30, 2020

danielshahaf commented May 31, 2020 via email • edited Loading

ericfreese commented May 31, 2020

danielshahaf commented Jun 1, 2020 via email

danielshahaf commented Jun 6, 2020

danielshahaf commented Jun 17, 2020 • edited Loading

danielshahaf commented Jun 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielshahaf commented Jun 18, 2020

danielshahaf commented Jun 25, 2020

okapia May 21, 2020 •

edited

Loading

danielshahaf commented May 31, 2020 via email •

edited

Loading

danielshahaf commented Jun 17, 2020 •

edited

Loading